mcp: reject relative paths in tool handlers#3694
Conversation
…ed tests Signed-off-by: Shrey327 <shreyansh.pathak273@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Shrey327 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
knative-prow
Bot
added
the
size/L
|
Welcome @Shrey327! It looks like this is your first PR to knative/func 🎉 |
knative-prow
Bot
added
the
needs-ok-to-test
|
Hi @Shrey327. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
lkingland
left a comment
lkingland
left a comment
There was a problem hiding this comment.
Thanks for the PR. Just two small suggestions.
| func validatePath(path string) error { | ||
| if !filepath.IsAbs(path) { | ||
| return fmt.Errorf("path must be absolute, got %q", path) | ||
| } |
There was a problem hiding this comment.
Consider a more agent-friendly error message — the client is usually an LLM. Something like:
return fmt.Errorf("path must be absolute (the MCP server's working directory is not the user's project root), got %q", path)The parenthetical hints at the why, which helps the agent fix its call instead of just retrying with a different relative path.
There was a problem hiding this comment.
Thanks for your review i have made the changes
| } | ||
| if err := validatePath("."); err == nil { | ||
| t.Fatal("expected error for '.', got nil") | ||
| } |
There was a problem hiding this comment.
Worth adding handler-level rejection tests so the wiring is covered, not just the helper. Something like:
func TestTool_Deploy_RejectsRelativePath(t *testing.T) {
client, _, err := newTestPair(t)
if err != nil { t.Fatal(err) }
result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "deploy",
Arguments: map[string]any{"path": "."},
})
if err != nil { t.Fatal(err) }
if !result.IsError { t.Fatal("expected error for relative path") }
}One per handler is verbose but mechanical. Alternatively, a table-driven test parameterized over tool names would cover all of them in one place.
There was a problem hiding this comment.
made the changes , i used the table-driven approach
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3694 +/- ##
==========================================
+ Coverage 56.90% 57.01% +0.10%
==========================================
Files 181 181
Lines 20933 20973 +40
==========================================
+ Hits 11912 11957 +45
+ Misses 7811 7808 -3
+ Partials 1210 1208 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Shrey327 <shreyansh.pathak273@gmail.com>
|
/ok-to-test |
knative-prow
Bot
added
ok-to-test
3 participants
Description
Adds
validatePath()to the MCP server that rejects non-absolute path arguments before shelling out to the func CLI. Sincecmd.Diris never set in the executor, relative paths silently resolved against the MCP server's working directory instead of the user's project turning a wrong directory failure into a silent misbehavior. This change turns that into an actionable error, matching the guidance already documented in instructions.md.All existing tests updated to uset.TempDir()and aTestValidatePathunit test added.